Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a bug in GetOverlappingInputsRangeBinarySearch #5211

Closed
wants to merge 1 commit into from
Closed

Fix a bug in GetOverlappingInputsRangeBinarySearch #5211

wants to merge 1 commit into from

Conversation

riversand963
Copy link
Contributor

@riversand963 riversand963 commented Apr 17, 2019 •
edited
Loading

As title.
We pass an argument next_smallest of type InternalKey** to GetOverlappingInputsRangeBinarySearch. In one place inside the function, we perform the following

if (next_smallest) {
  next_smallest = nullptr;
}

This does not have any effect after the function returns. Instead we should do *next_smallest = nullptr.

Test Plan:

$make clean && COMPILE_WITH_ASAN=1 make -j32 all && sleep 1 && make check

@riversand963
Fix a bug in GetOverlappingInputsRangeBinarySearch
22e0193 
Test Plan:
```
$make clean && COMPILE_WITH_ASAN=1 make -j32 all && sleep 1 && make check
```
@riversand963 riversand963 requested a review from anand1976 April 17, 2019 22:44
facebook-github-bot
facebook-github-bot reviewed Apr 18, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@riversand963 riversand963 deleted the fix_minor_bug branch April 18, 2019 16:39
riversand963 added a commit that referenced this pull request Apr 18, 2019
@riversand963
Fix a bug in GetOverlappingInputsRangeBinarySearch (#5211)
d81aedf 
Summary:
As title.
Pull Request resolved: #5211

Differential Revision: D14992018

Pulled By: riversand963

fbshipit-source-id: b5720ea4742029e2fb47ff6d9f8d9de006db4ed4
@facebook-github-bot
Copy link
Contributor

@riversand963 merged this pull request in 392f6d4.

@siying
Copy link
Contributor

siying commented Apr 18, 2019

Can you explain the impact of the bug in the title, and when it was introduced? If it is something already released, also mention it in HISTORY.md.

@riversand963
Copy link
Contributor Author

@siying @ajkr This is the fix mentioned in #5215
Now I kind of suspect I fixed some dead code...I may be totally wrong, so please let me know.
If you look at CompactionPicker::CompactRange. It calls VersionStorageInfo::GetOverlappingInputs(begin, end) first. If there is no overlapping file, it should return immediately. Only when there is an overlap will execution continue. Later it calls ExpandInputsToCleanCut which calls GetOverlappingInputs with key range that at least include [begin, end]. It seems certain that we will find file(s) overlapping. Therefore the line I changed will never be reached.

@siying
Copy link
Contributor

siying commented Apr 19, 2019

I know it's the fix mentioned in #5215 but still HISTORY.md should provide a better description.

If it is a dead code, it shouldn't be there and add an assert.

@riversand963
Copy link
Contributor Author

riversand963 commented Apr 19, 2019 •
edited
Loading

@siying I should not have used the term 'dead code'. If you pass different arguments to GetOverlappingInputs it is possible to hit this line. Therefore, I don't think we can put an assert.
I have not been able to create a case to show this bug either.

@siying
Copy link
Contributor

siying commented Apr 19, 2019

@riversand963 just update HISTORY.md to be specific about what the bug is, how it can be triggered and what's the impact.

vagogte pushed a commit to vagogte/rocksdb that referenced this pull request Jun 18, 2019
@riversand963 @facebook-github-bot
Fix a bug in GetOverlappingInputsRangeBinarySearch (facebook#5211)
10bc85e 
Summary:
As title.
Pull Request resolved: facebook#5211

Differential Revision: D14992018

Pulled By: riversand963

fbshipit-source-id: b5720ea4742029e2fb47ff6d9f8d9de006db4ed4
junhanLee95 pushed a commit to junhanLee95/rocksdb that referenced this pull request Oct 24, 2022
@riversand963
Fix a bug in GetOverlappingInputsRangeBinarySearch (facebook#5211)
d1b9655 
Summary:
As title.
Pull Request resolved: facebook#5211

Differential Revision: D14992018

Pulled By: riversand963

fbshipit-source-id: b5720ea4742029e2fb47ff6d9f8d9de006db4ed4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@anand1976 anand1976 anand1976 approved these changes

@miasantreble miasantreble miasantreble approved these changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@riversand963 @facebook-github-bot @siying @miasantreble @anand1976